Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Elasticsearch query runner #4391

Closed
wants to merge 1 commit into from
Closed

New Elasticsearch query runner #4391

wants to merge 1 commit into from

Conversation

NicolasLM
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor

Description

New implementation of the Elasticsearch query runner to overcome the limitations of the previous one.
The new runner is split into three:

  1. A runner supporting the newest versions of ES, aggregation, nested aggregations and nested fields.
  2. A runner for the SQL OpenDistro flavor
  3. A runner for the SQL X-Pack flavor

Related Tickets & Documents

Fixes #4293

The new elasticsearch query runner is split into three:

- A runner supporting the newest versions of ES,
  aggregation, nested aggregations and nested fields.
- A runner for the SQL OpenDistro flavor
- A runner for the SQL X-Pack flavor

Fixes #4293
@otto-von-bivouac
Copy link

Thanks for posting this PR, it is an awesome addition to redash.

I made a try with this change using ElasticSearch 6.8.2. Regardless what input type I choose (ElasticSearch2 or XPackSQLElasticSearch) the UI Reports 'Schema refresh failed.' when I try to query the data source. The original ElasticSearch datasource works well with this ES Instance.

I do not find any error in the error logs, even though the log-level is set to DEBUG:

server_1         | [2020-01-01 16:02:45,956][PID:13][DEBUG][metrics] table=groups query=select duration=23.58
server_1         | [2020-01-01 16:02:45,960][PID:13][DEBUG][urllib3.connectionpool] Starting new HTTP connection (1): host.docker.internal:9200
server_1         | [2020-01-01 16:02:45,991][PID:13][DEBUG][urllib3.connectionpool] http://host.docker.internal:9200 "GET /_mappings HTTP/1.1" 200 2503
server_1         | [2020-01-01 16:02:45,996][PID:13][INFO][metrics] method=GET path=/api/data_sources/3/schema endpoint=datasourceschemaresource status=200 content_type=application/json content_length=61 duration=77.99 query_count=6 query_duration=37.05
server_1         | [2020-01-01 16:02:46,000][PID:13][INFO][werkzeug] 172.24.0.1 - - [01/Jan/2020 16:02:46] "GET /api/data_sources/3/schema HTTP/1.1" 200 -
server_1         | [2020-01-01 16:02:46,874][PID:13][DEBUG][metrics] table=organizations query=select duration=2.00

GET /api/data_sources/3/schema returns:
{"error": {"code": 2, "message": "Error retrieving schema."}}

Any advice how to resolve this problem?

for index_name in mappings_data:
mappings[index_name] = {}
index_mappings = mappings_data[index_name]
_parse_properties('', index_mappings['mappings']['properties'])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to iterate through on all the documents within the index to parse the properties.

for m in index_mappings.get("mappings", {}):
    _parse_properties('', index_mappings['mappings'][m]['properties'])

For details see: #4391 (comment)

def _parse_results(cls, result_fields, raw_result):
error = raw_result.get('error')
if error:
raise Exception(error['reason'])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into this branch and it eat the original exception: error was a string for me, which is not addressable by 'reason'.
Raising the error solved the problem: raise Exception(error)

query, url, result_fields = self._build_query(query)
response, error = self.get_response(
url,
http_method='post',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POST method is only allowed with generic ElasticSearch queries and not with SQL.
For SQL queries PUT has to be set, which is not compatible with the generic queries.

Therefore this field should be factored into a variable which is settable per query type.

@dgkncelik
Copy link

Hi, can i work on this? I also need that feature :)

itssimon added a commit to lumonus/redash that referenced this pull request Jul 14, 2020
@wwl717195673
Copy link
Contributor

How can i use this feature?I am confused.

@susodapop
Copy link
Contributor

These changes (and commit history) are incorporated into #5794. Closing here.

@susodapop susodapop closed this Jul 13, 2022
@guidopetri guidopetri deleted the new-elasticsearch branch July 22, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Elasticsearch query runner
8 participants